SSE_C changes by rajdchak · Pull Request #281 · awslabs/analytics-accelerator-s3

rajdchak · 2025-06-03T16:21:02Z

Description of change

Support passing of sse_c customer key to pass this to s3 for encryption/decryption.

S3A currently has this customer key as Optional String https://github.com/apache/hadoop/blob/trunk/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/delegation/EncryptionSecretOperations.java#L41 while Iceberg has this key as a String https://github.com/apache/iceberg/blob/f9cc62eb0d98e360b452a3ab8fdc6efdc4969f6e/aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java#L499. So decided to accept this key as Optional String.

Relevant issues

Does this contribution introduce any breaking changes to the existing APIs or behaviors?

Does this contribution introduce any new public APIs or behaviors?

How was the contribution tested?

Does this contribution need a changelog entry?

I have updated the CHANGELOG or README if appropriate

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

object-client/src/main/java/software/amazon/s3/analyticsaccelerator/S3SdkObjectClient.java

common/build.gradle.kts

fuatbasik

Thanks @rajdchak . My major comment is around dependencies. We should not depend sdk.core for this change. We also should not add any S3 dependency to common.

common/build.gradle.kts

common/src/main/java/software/amazon/s3/analyticsaccelerator/request/EncryptionSecrets.java

gradle/libs.versions.toml

input-stream/src/testFixtures/java/software/amazon/s3/analyticsaccelerator/access/S3Object.java

common/src/main/java/software/amazon/s3/analyticsaccelerator/request/EncryptionSecrets.java

.../integrationTest/java/software/amazon/s3/analyticsaccelerator/access/SSECEncryptionTest.java

common/src/main/java/software/amazon/s3/analyticsaccelerator/request/EncryptionSecrets.java

...on/src/test/java/software/amazon/s3/analyticsaccelerator/util/OpenStreamInformationTest.java

ahmarsuhail

thanks, mostly looks good, have some minor comments

...on/src/test/java/software/amazon/s3/analyticsaccelerator/util/OpenStreamInformationTest.java

...integrationTest/java/software/amazon/s3/analyticsaccelerator/access/IntegrationTestBase.java

...tFixtures/java/software/amazon/s3/analyticsaccelerator/access/S3AsyncClientStreamReader.java

.../integrationTest/java/software/amazon/s3/analyticsaccelerator/access/SSECEncryptionTest.java

fuatbasik

@rajdchak thanks a lot for your PR. I put few comments. I have 2 big one concerns we need to re-do.

1/ Please do not share SSE-C key value in plain-text. We can get them from an environment variable but we shouldn't get share them here like this. I think the way it should work is if ENV_VAR has SSE-C not set, it should skip these tests othewise execute them.

2/ Your change breaks integration tests data generation logic today, the tests will fail if a new developer just follows the DEVELOPMENT.MD. Please make sure you are not failing this steps when updating integration tests.

3/ Could you please add examples to README on how to enable SSE-C and how to use it.

.../integrationTest/java/software/amazon/s3/analyticsaccelerator/access/SSECEncryptionTest.java

fuatbasik · 2025-06-09T12:25:48Z

.../integrationTest/java/software/amazon/s3/analyticsaccelerator/access/SSECEncryptionTest.java

+
+  static Stream<Arguments> encryptedParquetReads() {
+    List<S3Object> readEncryptedObjects = new ArrayList<>();
+    readEncryptedObjects.add(S3Object.RANDOM_SSEC_ENCRYPTED_PARQUET_1MB);


how these two objects are generated and uploaded to S3 bucket?

...on/src/test/java/software/amazon/s3/analyticsaccelerator/util/OpenStreamInformationTest.java

input-stream/src/testFixtures/java/software/amazon/s3/analyticsaccelerator/access/S3Object.java

.../integrationTest/java/software/amazon/s3/analyticsaccelerator/access/SSECEncryptionTest.java

ahmarsuhail

Thanks @rajdchak , looks good to me overall.

Same question as @fuatbasik, I'm not familiar with how we do data generation for files we use for our ITests. have we updated that logic so it's possible to create these new files?

ahmarsuhail

+1, LGTM.

test file generation to be addressed in a follow up.

fuatbasik · 2025-06-11T19:28:54Z

...integrationTest/java/software/amazon/s3/analyticsaccelerator/access/IntegrationTestBase.java

      S3AsyncClient s3Client = this.getS3ExecutionContext().getS3Client();
-      S3SeekableInputStream stream = s3AALClientStreamReader.createReadStream(s3Object);
+      S3SeekableInputStream stream =
+          s3AALClientStreamReader.createReadStream(s3Object, OpenStreamInformation.DEFAULT);


you could've saved yourself lots of time keeping createReadStream(s3Object) method around and inside call createReadStream(s3Object, OpenStreamInformation.DEFAULT).

This is a nit btw but probably a better way to implement this pattern.

fuatbasik · 2025-06-11T19:31:28Z

...src/testFixtures/java/software/amazon/s3/analyticsaccelerator/access/S3StreamReaderBase.java

   * @param checksum optional checksum, to update
+   * @param openStreamInformation contains the open stream information
   */
  public abstract void readPattern(


nit: do we want to change existing method signature or add a new method?

object-client/src/main/java/software/amazon/s3/analyticsaccelerator/S3SdkObjectClient.java

## Description of change    Support passing of sse_c customer key to pass this to s3 for encryption/decryption. S3A currently has this customer key as Optional String https://github.com/apache/hadoop/blob/trunk/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/auth/delegation/EncryptionSecretOperations.java#L41 while Iceberg has this key as a String https://github.com/apache/iceberg/blob/f9cc62eb0d98e360b452a3ab8fdc6efdc4969f6e/aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java#L499. So decided to accept this key as Optional String. #### Relevant issues   #### Does this contribution introduce any breaking changes to the existing APIs or behaviors?  #### Does this contribution introduce any new public APIs or behaviors?  #### How was the contribution tested?  #### Does this contribution need a changelog entry? - [ ] I have updated the CHANGELOG or README if appropriate --- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the [Developer Certificate of Origin (DCO)](https://developercertificate.org/).

rajdchak had a problem deploying to integration-tests June 3, 2025 16:21 — with GitHub Actions Failure

ahmarsuhail reviewed Jun 4, 2025

View reviewed changes

object-client/src/main/java/software/amazon/s3/analyticsaccelerator/S3SdkObjectClient.java Outdated Show resolved Hide resolved

rajdchak force-pushed the sse-c branch from b8b11ae to 5eaa287 Compare June 5, 2025 14:24

rajdchak had a problem deploying to integration-tests June 5, 2025 14:24 — with GitHub Actions Failure

rajdchak had a problem deploying to integration-tests June 5, 2025 14:35 — with GitHub Actions Failure

rajdchak changed the title ~~SSE_C initial changes~~ SSE_C changes Jun 5, 2025

rajdchak marked this pull request as ready for review June 5, 2025 14:36

fuatbasik reviewed Jun 5, 2025

View reviewed changes

common/build.gradle.kts Outdated Show resolved Hide resolved

rajdchak had a problem deploying to integration-tests June 5, 2025 14:47 — with GitHub Actions Failure

fuatbasik reviewed Jun 5, 2025

View reviewed changes

rajdchak force-pushed the sse-c branch from d0d43fb to ab3d4ea Compare June 5, 2025 16:28

rajdchak had a problem deploying to integration-tests June 5, 2025 16:28 — with GitHub Actions Failure

SSE_C support

d7d3609

rajdchak force-pushed the sse-c branch from ab3d4ea to d7d3609 Compare June 5, 2025 16:48

rajdchak had a problem deploying to integration-tests June 5, 2025 16:48 — with GitHub Actions Failure

SSE_C support

70bd9d4

rajdchak had a problem deploying to integration-tests June 5, 2025 16:53 — with GitHub Actions Failure

exception

809475a

rajdchak had a problem deploying to integration-tests June 5, 2025 17:08 — with GitHub Actions Failure

ahmarsuhail reviewed Jun 6, 2025

View reviewed changes

common/src/main/java/software/amazon/s3/analyticsaccelerator/request/EncryptionSecrets.java Show resolved Hide resolved

ahmarsuhail reviewed Jun 6, 2025

View reviewed changes

.../integrationTest/java/software/amazon/s3/analyticsaccelerator/access/SSECEncryptionTest.java Outdated Show resolved Hide resolved

ahmarsuhail reviewed Jun 6, 2025

View reviewed changes

common/src/main/java/software/amazon/s3/analyticsaccelerator/request/EncryptionSecrets.java Show resolved Hide resolved

ahmarsuhail reviewed Jun 6, 2025

View reviewed changes

...on/src/test/java/software/amazon/s3/analyticsaccelerator/util/OpenStreamInformationTest.java Show resolved Hide resolved

ahmarsuhail requested changes Jun 6, 2025

View reviewed changes

Fix in integ tests

2147905

rajdchak had a problem deploying to integration-tests June 6, 2025 15:29 — with GitHub Actions Failure

Added parquet test

314ede6

rajdchak had a problem deploying to integration-tests June 9, 2025 10:21 — with GitHub Actions Failure

Added parquet test

af9204c

rajdchak temporarily deployed to integration-tests June 9, 2025 10:53 — with GitHub Actions Inactive

rajdchak requested a review from ahmarsuhail June 9, 2025 11:26

fuatbasik reviewed Jun 9, 2025

View reviewed changes

input-stream/src/testFixtures/java/software/amazon/s3/analyticsaccelerator/access/S3Object.java Outdated Show resolved Hide resolved

fuatbasik reviewed Jun 9, 2025

View reviewed changes

.../integrationTest/java/software/amazon/s3/analyticsaccelerator/access/SSECEncryptionTest.java Outdated Show resolved Hide resolved

fuatbasik reviewed Jun 9, 2025

View reviewed changes

.../integrationTest/java/software/amazon/s3/analyticsaccelerator/access/SSECEncryptionTest.java Show resolved Hide resolved

awslabs deleted a comment from ahmarsuhail Jun 9, 2025

Added parquet test

d7e58ca

rajdchak temporarily deployed to integration-tests June 9, 2025 20:21 — with GitHub Actions Inactive

Added parquet test

856d741

rajdchak temporarily deployed to integration-tests June 9, 2025 23:31 — with GitHub Actions Inactive

readme

751e97b

rajdchak had a problem deploying to integration-tests June 10, 2025 09:00 — with GitHub Actions Failure

update test

1080fd9

rajdchak temporarily deployed to integration-tests June 10, 2025 10:00 — with GitHub Actions Inactive

ahmarsuhail reviewed Jun 11, 2025

View reviewed changes

ahmarsuhail approved these changes Jun 11, 2025

View reviewed changes

fuatbasik reviewed Jun 11, 2025

View reviewed changes

object-client/src/main/java/software/amazon/s3/analyticsaccelerator/S3SdkObjectClient.java Show resolved Hide resolved

rajdchak merged commit 1343077 into awslabs:main Jun 11, 2025
4 checks passed

Conversation

rajdchak commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of change

Relevant issues

Does this contribution introduce any breaking changes to the existing APIs or behaviors?

Does this contribution introduce any new public APIs or behaviors?

How was the contribution tested?

Does this contribution need a changelog entry?

Uh oh!

Uh oh!

Uh oh!

fuatbasik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahmarsuhail left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fuatbasik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fuatbasik Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahmarsuhail left a comment

Choose a reason for hiding this comment

Uh oh!

ahmarsuhail left a comment

Choose a reason for hiding this comment

Uh oh!

fuatbasik Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

fuatbasik Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

fuatbasik Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rajdchak commented Jun 3, 2025 •

edited

Loading

fuatbasik Jun 11, 2025 •

edited

Loading